hardening: migrate flowview process/version lookups to prepared - #248
Merged
TheWitness merged 11 commits intoSep 15, 2026
Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Updates FlowView plugin code to replace raw SQL string queries with prepared statements, and adds a lightweight regression test to detect reintroduction of those raw lookups.
Changes:
- Converted plugin version lookup in
setup.phptodb_fetch_cell_prepared()with a bound placeholder. - Converted FlowView master process PID lookups in
setup.phpandflowview_devices.phptodb_fetch_cell_prepared()with bound placeholders. - Added a PHP test script that scans source files to assert prepared-statement usage and absence of specific raw lookups.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| tests/test_prepared_statements.php | Adds string-based regression checks to enforce prepared-statement usage for specific queries. |
| setup.php | Replaces raw db_fetch_cell() lookups with db_fetch_cell_prepared() for plugin version and PID retrieval. |
| flowview_devices.php | Replaces raw PID lookups with prepared statements for save/restart service paths. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
somethingwithproof
changed the title
Member
Author
|
Incorporated follow-up review feedback in |
somethingwithproof
marked this pull request as draft
March 20, 2026 10:00
Member
|
Looks like after the last merge, you should update your branch @somethingwithproof |
Add targeted tests for prepared statement migration, output escaping, auth guard presence, CSRF token validation, redirect safety, and PHP 7.4 compatibility. Tests use source-scan patterns that verify security invariants without requiring the Cacti database. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
somethingwithproof
force-pushed
the
fix/prepared-lookups-247
branch
from
July 29, 2026 05:35
2ffba84 to
acea08d
Compare
TheWitness
requested changes
Sep 8, 2026
TheWitness
left a comment
TheWitness
left a comment
Member
There was a problem hiding this comment.
No composer.json in plugins.
xmacan
previously approved these changes
Sep 15, 2026
# Conflicts: # tests/Pest.php # tests/Security/Php74CompatibilityTest.php # tests/Security/SetupStructureTest.php
- drop composer.json and tests/bootstrap.php; Pest and the Cacti API stubs now come from tests/bootstrap-unit.php via phpunit.xml, same as every other suite in this repo - move the E2E and Integration regression tests into tests/Security, since they are static source-scan checks like the rest of that suite and phpunit.xml only scans tests/Security and tests/Unit
…acti#239) get_ip_filter() masked VARBINARY addresses with a bitwise AND, which MySQL 8.0 evaluates correctly but MariaDB casts both sides to BIGINT and always matches, silently returning every row. Replace it with a LENGTH()-guarded BETWEEN range, which is portable across both servers and keeps a 4-byte IPv4 range from reaching 16-byte IPv6 rows. Reported and diagnosed by @somethingwithproof in Cacti#239, whose suggested fix this applies verbatim. Closes Cacti#239
…harness Verified with a sandbox Cacti checkout (Pest 3 + phpunit.xml from Cacti#260): - AuthGuardTest/OutputEscapingTest/RedirectSafetyTest/PreparedStatementConsistencyTest scanned tests/test_prepared_statements.php itself as if it were a UI entry point, which isn't meaningful for a test script; drop it from those scan lists. - PreparedStatementConsistencyTest flagged setup.php's two db_execute() calls that create the core Cacti reports_log/reports_queued tables. Those are static CREATE TABLE DDL with no bound values, so there is nothing to parameterize; exclude DDL lines from the raw-call check. All 61 tests in tests/Security and tests/Unit now pass.
TheWitness
approved these changes
Sep 15, 2026
bmfmancini
approved these changes
Sep 15, 2026
bmfmancini
left a comment
bmfmancini
left a comment
Member
There was a problem hiding this comment.
Approving — the prepared-statement migration for the process/version lookups, the port normalization before shell_exec, and the MariaDB-portable LENGTH()/BETWEEN CIDR range predicate in get_ip_filter() all look correct. Regression tests cover the changes well.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Implements issue #247 by migrating selected raw process/version lookups to prepared database helpers, and fixes #239 by replacing a non-portable bitwise CIDR match in
get_ip_filter()with a range comparison.Changes
flowview_devices.phpsave_device()todb_fetch_cell_prepared()restart_services()todb_fetch_cell_prepared()setup.phpplugin_flowview_check_config()todb_fetch_cell_prepared()flowview_global_settings_update()todb_fetch_cell_prepared()functions.phpget_ip_filter()matched CIDR ranges with a bitwise AND against aVARBINARYcolumn, which MySQL 8.0 evaluates correctly but MariaDB casts toBIGINTand always matches, so every row passed the filter. Replaced with aLENGTH()-guardedBETWEENrange comparison, portable across both servers, per @somethingwithproof's diagnosis and suggested fix in Flowview not taking account of the filter src/dst IP #239.tests/test_prepared_statements.phptests/Unit/QueryBuilderTest.phpupdated for the new CIDR range predicate/paramsValidation
php -l flowview_devices.phpphp -l setup.phpphp -l tests/test_prepared_statements.phpphp tests/test_prepared_statements.phpIssue
Closes #247
Closes #239